fetchFromGitHub: add archiveReproducibilityWorkaround option#428377
fetchFromGitHub: add archiveReproducibilityWorkaround option#428377YorikSar wants to merge 2 commits intoNixOS:masterfrom
Conversation
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
Can we get away with not doing the replacements at all for simplicity? A |
roberth
left a comment
There was a problem hiding this comment.
This looks like a good strategy to solve the export-subst problem.
Could you:
- factor out the processing logic into a separate script package with a test, and/or
- add a fetcher test like we have for fetchgit
Line 133 in 1f91f2e
Those use https://nixos.org/manual/nixpkgs/unstable/#tester-invalidateFetcherByDrvHash to generate unique FODs per fetcher implementation "version". Otherwise you end up testing nothing at all after running it once.
Unfortunately, we don't have any for fetchFromGitHub yet. Yikes.
We won't be able to change its behavior easily when this is in use, so there's some reality to that fear. I think it can be done, if we have tests. Future iterations of it may need to be versioned, because otherwise we'd break existing call sites' FODs. |
|
Do we know for sure that we need the processing logic? Most things don’t use this and will build fine from a Git clone, which is what fetching the tree with no post‐processing would give us. We could have a separate hook, that runs outside the FOD (and is therefore not such a compatibility risk), but reuses information from |
0a4d73c to
c928acd
Compare
YorikSar
left a comment
There was a problem hiding this comment.
@roberth, I've addressed all your comments, except I didn't move the script to a separate package yet, will do this later. I'll add a TODO list to the top post.
@emilazy While some packages don't really use this, packages with user interface (CLI, WebUI, etc) might display version string, and it will show the template instead of the actual version. While we could fix this package by package as we currently do, I agree with @roberth's comment that this warrants a more generic fix.
But there’s nothing stopping us only implementing the raw fetch inside (Even then I’d prefer to wait to see if anything warrants it – we already do a lot of patching of packages for bad version checks and it’s not clear to me this would be a meaningful additional burden – but it wouldn’t have the serious issues of putting more logic in FODs, something we’re trying to get away from as it keeps causing us issues.) |
|
@emilazy A hook would need the tag name to be passed as an environment variable. It would be nicer to have a function that produces a finished "git archive"-like store path. You're absolutely right to criticize FOD complexity and I probably shouldn't have made an exception for this.
These could then be created by a single function, to give it a nice interface, e.g. |
911a04a to
c14210c
Compare
This option fetches tarball from GitHub based on tree hash instead of the tag to get unprocessed data from the repo, and then applies a partial implementation of `export-subst` to replace certain format strings, but unlike Git uses reproducible values for them. This includes an example of using this in a package affected by this non-reproducibility. Related PR: NixOS#417859 Related issue: NixOS#84312 Inspired by discussion: https://discourse.nixos.org/t/fetchfromgithub-and-the-versioneer-fixing-source-reproducibility/66539
c14210c to
72299b3
Compare
|
I've extracted the substitution logic into a separate package and rebased the branch on top of master to resolve a conflict. Could you please point me to the argument against adding logic to FOD? We seem to run rather complex computations in them anyway (e.g. any I'm not against moving this outside of FODs, but there would be a problem of getting data for |
|
I'm just scared of this PR in general. I would prefer (in general) that these sorts of features are turned off at some high level, and then when that choice to opt out becomes a problem, we make a specific fetcher for the specific problem at hand. That causes me much less fear. |
This option fetches tarball from GitHub based on tree hash instead of the tag to get unprocessed data from the repo, and then applies a partial implementation of
export-substto replace certain format strings, but unlike Git uses reproducible values for them.This includes an example of using this in a package affected by this non-reproducibility.
Related PR: #417859
Related issue: #84312
Inspired by discussion: https://discourse.nixos.org/t/fetchfromgithub-and-the-versioneer-fixing-source-reproducibility/66539
TODO
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.